Update expression resolver to return null instead of blank string. #296

mattcoley · 2019-03-05T21:15:30Z

It doesn't make sense to return an empty string from the expression resolver if the expression cannot be evaluated due to an exception. Some built-in tags will treat "" differently from null. For example for will treat "" as [""] whereas it should be treated as [].

mattcoley · 2019-03-05T21:19:17Z

I could not find a reason why we return empty string here. We used to return false and then that was updated to empty string. The tags that use the expression resolver handle the null value better than empty string.

jboulter

When was it updated? The only the reason I can think of is that if you're echoing the result back out, we'd rather return an empty string to the page rather than "null".

codecov-io · 2019-03-05T21:26:14Z

Codecov Report

Merging #296 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #296      +/-   ##
============================================
+ Coverage     71.96%   71.98%   +0.02%     
- Complexity     1525     1526       +1     
============================================
  Files           237      237              
  Lines          4769     4769              
  Branches        757      757              
============================================
+ Hits           3432     3433       +1     
  Misses         1071     1071              
+ Partials        266      265       -1

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/hubspot/jinjava/el/ExpressionResolver.java	`81.96% <100%> (ø)`	`16 <0> (ø)`	⬇️
.../main/java/com/hubspot/jinjava/lib/tag/ForTag.java	`87.3% <0%> (+1.58%)`	`20% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a807379...ebcc216. Read the comment docs.

mattcoley · 2019-03-05T21:33:04Z

Here: 1d694cd. The expression resolver is only used in tags: do, for, if, macro, etc. and in the ExpressionNode https://github.com/HubSpot/jinjava/blob/master/src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java#L42. The expression node handles the null value fine by defaulting to empty string.

Matt Coley added 3 commits March 5, 2019 16:10

Update expression resolver to return null instead of blank string.

1eaac21

slight fix.

de9a974

update tests.

ebcc216

mattcoley requested a review from boulter March 5, 2019 21:19

jboulter reviewed Mar 5, 2019

View reviewed changes

mattcoley merged commit 4c5b1c8 into master Mar 6, 2019

mattcoley deleted the update-resolver branch March 6, 2019 15:35

mattcoley mentioned this pull request Mar 6, 2019

Revert "Update expression resolver to return null instead of blank string." #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update expression resolver to return null instead of blank string. #296

Update expression resolver to return null instead of blank string. #296

mattcoley commented Mar 5, 2019

mattcoley commented Mar 5, 2019

jboulter left a comment

codecov-io commented Mar 5, 2019

mattcoley commented Mar 5, 2019

Update expression resolver to return null instead of blank string. #296

Update expression resolver to return null instead of blank string. #296

Conversation

mattcoley commented Mar 5, 2019

mattcoley commented Mar 5, 2019

jboulter left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 5, 2019

Codecov Report

mattcoley commented Mar 5, 2019